-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Converted the player palette to an enum class #303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you! Just 2 minor things, otherwise looks good.
src/smw/player.h
Outdated
@@ -90,6 +90,7 @@ class CPlayer | |||
short getGlobalID() const { return globalID; } | |||
short getTeamID() const { return teamID; } | |||
short getColorID() const { return colorID; } | |||
PlayerPalette getPlayerPalette(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
player.animationstate += 32; | ||
if (player.animationstate > 96) | ||
player.animationstate++; | ||
if (player.animationstate > 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the new enum values here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that you managed to remove the separate animation fields. There are some readability questions about the implementation.
return invincibility_animation_states[(timer / 4) % 4]; | ||
} else { | ||
return invincibility_animation_states[((timer - 4) / 7) % 4]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could make the indices more readable, that would explain why these numbers are used? for example,
- If you use
std::array
for the animation states, you can then use itssize()
in the% 4
part - The
/ 4
and/ 7
parts seem to control the speed of the animation – you could move them to a named variable - Why do we subtract
- 4
in the second branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this looks good, thanks!
No description provided.